Do you know why this position changed?

I'm not sure I understand why this error message had the position that it did in the first place. It is still pointing to the same interface field at least.

The message is about the definition for out in the module (at line 10, though referring to the use of y at line 11). However, what is happening under the covers is that BSC is creating an interface struct from all the definitions, and the names for the fields of that struct are taken from the interface type declaration, and out is declared at line 2. So that causes an error on out in the module to end up pointing to out in the type. It's a known issue that we should fix at some point (Bluespec Inc internal bug database issue 1238).

However, it's interesting that your PR has changed the position from the name (out) to the type (Bit#(8)). It could be due to how you're picking positions for any ISyntax/CSyntax constructs that you're building, or maybe how you've changed the expression structures (causing getPosition to find a different position first). I'll try to keep an eye out for it while looking through your changes. (I do think it's worth understanding why positions change -- regardless of what we think of the specific test example, because it could show up as a position change in other situations that we're not testing.)

I spent a while looking at this, and I'm having a hard time figuring out what is going on here. As far as I can tell, this id is coming from the name in the FInf built by chkInterface, which has the type as its position. I'm not sure how this previously had a different position.

Note that the testcase is compiled with -cross-info. Without that flag, the message doesn't display a position. So it's something in the -cross-info mechanism that is inserting the better position (and maybe the default behavior, without the flag, hasn't changed).

This is a flag/mechanism that I would like to excise, and replace with good position tracking by default. Given that, I think it's OK to not worry about the change now. If it's a problem, we can fix it later.

However, if you feel like looking to see how the -cross-info flag was working before, and whether that position could be preserved somehow now (either with or without the flag), that'd be welcome.

Maybe we should at least add a second text to the exp file that compares the output of the test when compiled without the flag, too, just to check for regressions in that behavior, too.

I don't really expect to have time to keep investigating this soon, but I added a test to compare the output without --cross-info.

Hm, would the size of (x,()) be reported as 1? Do we think it should be 2?

Hmm, I suppose it would be reported as 1.

I don't think it's too unreasonable to treat tuples that end in () as one element smaller. Note that AppendTuple also would drop the () if the first tuple being appended ends in (), and the same for CurryN.

I recall that there is a general issue with tuples: because they are build from nested pairs, the last element can't be of a tuple type, otherwise it's indistinguishable from one large tuple. For example (and this works in BSV too), if you have a variable of type Tuple2 a (Tuple2 b c), you can assign to it with tuple3 -- and the current implementation of TupleSize considers it 3. Here's code that you can try:

package TupleSizeBH where

type T3 = Tuple2 Bool (Tuple2 Bool Bool)

mkTupleSizeBH :: (TupleSize T3 3) => Module Empty
mkTupleSizeBH =
  module
    let x :: T3
        x = tuple3 True False True

However, if we have a variable of type Tuple3 a b (), that cannot be assigned with tuple2 -- BSC gives a type error unless you use tuple3. However TupleSize says its 2. This inconsistency seems wrong to me.

The issue is that the expression(e1,e2) parses into PrimPair e1 e2, and (e) parses the same as e, and () parses into PrimUnit. Longer tuples are nestings of PrimPair, but PrimUnit is not a zero of PrimPair -- this is perhaps clearer in BSV, where you can't write (), it has to be written as void.

The inconsistency can be fixed for TupleSize by defining it like this:

instance TupleSize a 1 where {}
instance TupleSize (a,b) 2 where {}
instance (TupleSize b n) => TupleSize (a, b) (TAdd n 1) where {}

But you're saying that AppendTuple and curryN also use PrimUnit as a zero. Here is an example showing that appending a Tuple3 and Tuple2 will give Tuple4 if the Tuple3 ends with PrimUnit:

package AppendTupleBH where

type TA = Tuple3 Bool Bool ()

type TB = Tuple2 Bool Bool

-- type TC = tuple5 Bool Bool () Bool Bool
type TC = Tuple4 Bool Bool Bool Bool

mkAppendTupleBH :: (AppendTuple TA TB TC) => Module Empty
mkAppendTupleBH =
  module
     let x :: TA
         x = tuple3 True False ()
         y :: TB
         y = tuple2 True False
         z :: TC
         z = appendTuple x y

This seems to be because you want to support () as a zero in calls to appendTuple and splitTuple, and so there need to be instances for (), but you have also written your typeclass so that it reuses the top-level instances for sub-elements of the tuple. If you separate the processing of the sub-elements from the top-level, I think the issue goes away. Of course, there's also a complication that BSC won't allow overlapping instance of the form T () x and T x (), even if a more explicit instance for the intersection T () () is given -- which is unfortunate, and maybe something that can be fixed one day? You got around that by adding a second level of typeclass. However, that second level is still conflating top-level () with sub-element (). I think you just need to add a third level of typeclass, that's only for processing once top-level () has been eliminated -- and in fact I confirmed that this works:

class AppendTuple a b c | a b -> c where
  appendTuple :: a -> b -> c
  splitTuple :: c -> (a, b)

instance AppendTuple a () a where
  appendTuple x _ = x
  splitTuple x = (x, ())

instance (AppendTuple' a b c) => AppendTuple a b c where
  appendTuple = appendTuple'
  splitTuple = splitTuple'

class AppendTuple' a b c | a b -> c where
  appendTuple' :: a -> b -> c
  splitTuple' :: c -> (a, b)

instance AppendTuple' () a a where
  appendTuple' _ x = x
  splitTuple' x = ((), x)

instance (AppendTuple'' a b c) => AppendTuple' a b c where
  appendTuple' = appendTuple''
  splitTuple' = splitTuple''

class AppendTuple'' a b c | a b -> c where
  appendTuple'' :: a -> b -> c
  splitTuple'' :: c -> (a, b)

-- Top-level () has been handled before AppendTuple''
-- so no instance is needed for () because occurrences
-- can only be sub-elements at this point and are handled
-- by the instance for sub-elements of any type

instance AppendTuple'' a b (a, b) where
  appendTuple'' a b = (a, b)
  splitTuple'' = id

instance (AppendTuple'' a b c) => AppendTuple'' (h, a) b (h, c) where
  appendTuple'' (x, y) z = (x, appendTuple'' y z)
  splitTuple'' (x, y) = case splitTuple'' y of
    (w, z) -> ((x, w), z)

I haven't looked closely at curryN, but I assume that a layer of hierarchy could be added, with () not handled in the second layer.

I was worried that there's still the issue that splitTuple of Tuple3 Bool Bool () to split off the last element and then appendTuple to put it back won't result in the original tuple. But actually, both definitions are allowed -- by your implementation and mine! Unless I'm missing some reason why this example compiles:

package TestSplitAppend where

type TA = Tuple3 Bool Bool ()

mkTestSplitAppend :: Module Empty
mkTestSplitAppend =
  module
     let x :: TA
         x = tuple3 True False ()
         p :: ((Bool,Bool), ())
         p = splitTuple x
         y1 :: Tuple2 Bool Bool
         y1 = case p of
                (a,b) -> appendTuple a b
         y2 :: TA
         y2 = case p of
                (a,b) -> appendTuple a b

     let e1 :: (Bool, Bool)
         e1 = (True, False)
         e2 :: ()
         e2 = ()
         e31 :: (Bool, Bool)
         e31 = appendTuple e1 e2
         e32 :: (Bool, Bool, ())
         e32 = appendTuple e1 e2

And it's not because the types are considered the same, because adding y1 == y2 causes a type error.

There's a dependency on the typeclass, so for the same arguments, only one result type should be possible:

class AppendTuple a b c | a b -> c where

Separately, isn't there something missing in the dependencies you declared? Shouldn't it also have a c -> b and b c -> a?

I'm just getting back to your comments here - I'm not sure I agree your version of TupleSize is what we want?
The version you specified would have TupleSize () 1, but we want () to have size 0 for the purposes of checkPortNames. I suppose we could introduce TupleSize' and make (a, ()) have size 2 but () have size 0 - would that address your concern?

I updated the TupleSize, AppendTuple and Curry type classes with an additional level of indirection to avoid treating (a, ()) as a tuple of size 1. Adding the extra reverse functional dependencies creates some ambigous instance errors, so I did not do that.

To be specific, adding the extra fundeps on AppendTuple and its helpers gives

Error: "Prelude.bs", line 3449, column 9: (T0128)
  Overlapping typeclass instances Prelude.AppendTuple'' a b (a, b) and
  Prelude.AppendTuple'' (h, a) b (h, c) (defined at "Prelude.bs", line 3453,
  column 34 ) cannot be ordered from most-specific to least-specific, so they
  cannot support predictable instance-resolution. Please resolve this by
  changing one or both of the instances.

The message is about the definition for out in the module (at line 10, though referring to the use of y at line 11). However, what is happening under the covers is that BSC is creating an interface struct from all the definitions, and the names for the fields of that struct are taken from the interface type declaration, and out is declared at line 2. So that causes an error on out in the module to end up pointing to out in the type. It's a known issue that we should fix at some point (Bluespec Inc internal bug database issue 1238).

However, it's interesting that your PR has changed the position from the name (out) to the type (Bit#(8)). It could be due to how you're picking positions for any ISyntax/CSyntax constructs that you're building, or maybe how you've changed the expression structures (causing getPosition to find a different position first). I'll try to keep an eye out for it while looking through your changes. (I do think it's worth understanding why positions change -- regardless of what we think of the specific test example, because it could show up as a position change in other situations that we're not testing.)